🔒 Fix SSRF vulnerability in Fetcher (DNS Rebinding)#40
Conversation
Replaces the vulnerable TOCTOU domain validation check with a robust connection-time validation using `secureDispatcher`. Explicit IP literals are still validated early using `net.isIP` and `IPGuard.isInternal` to fail fast, but domain names are now resolved and validated atomically by `undici`'s lookup hook, preventing DNS rebinding attacks. Fetcher now correctly catches `EBLOCKED` errors from the dispatcher and returns `blocked_internal_ip` status. Added `tests/fetcher_dispatcher.test.ts` to verify the fix and integration with `secureDispatcher` failure modes.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Replaces the vulnerable TOCTOU domain validation check with a robust connection-time validation using `secureDispatcher`. Explicit IP literals are still validated early using `net.isIP` and `IPGuard.isInternal` to fail fast, but domain names are now resolved and validated atomically by `undici`'s lookup hook, preventing DNS rebinding attacks. Fetcher now correctly catches `EBLOCKED` errors from the dispatcher and returns `blocked_internal_ip` status. Added `tests/fetcher_dispatcher.test.ts` to verify the fix and integration with `secureDispatcher` failure modes. Also fixed lint warnings in `transport.test.ts` and `crawler.ts`.
|
Closing as this was addressed by PR 42 which fixed the SSRF TOCTOU issue exactly the same way without the unnecessary lockfile bumps. |
1 participant
This PR fixes a Server-Side Request Forgery (SSRF) vulnerability caused by a Time-of-Check Time-of-Use (TOCTOU) race condition in the
Fetcherclass.🎯 Vulnerability
Previously,
Fetcherperformed a DNS resolution check (IPGuard.validateHost) before initiating the HTTP request. An attacker could exploit the time window between this check and the actual connection to rebind the DNS record to an internal IP (DNS Rebinding), bypassing the check.🛡️ Solution
The fix removes the pre-validation for domain names and relies entirely on the
secureDispatcher(which uses a customlookupfunction) to validate the IP address at the moment of connection establishment. This eliminates the race condition.net.isIPandIPGuard.isInternalto fail fast.undiciviasecureDispatcher.Fetchernow catchesEBLOCKEDerrors thrown byIPGuardand correctly reports them asblocked_internal_ip.✅ Verification
plugins/core/tests/fetcher_dispatcher.test.tswhich mocksundiciandIPGuardto simulate an internal IP block at connection time, verifying thatFetcherhandles it correctly and returnsblocked_internal_ip.PR created automatically by Jules for task 8560312622372837981 started by @saurabhsharma2u